Skip to content

top and head: allow limit keywords#4249

Merged
penghuo merged 16 commits into
opensearch-project:mainfrom
Swiddis:feature/headtop-limit
May 26, 2026
Merged

top and head: allow limit keywords#4249
penghuo merged 16 commits into
opensearch-project:mainfrom
Swiddis:feature/headtop-limit

Conversation

@Swiddis

@Swiddis Swiddis commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

Description

A small set of improvements:

  • Allows setting limit=N for head and top, similar to what timechart supports. Helps makes queries slightly more readable if storing them somewhere.
  • Adds limit to the available keyword field names (wasn't added by timechart).
  • Broke out AstPlanningTest to help split up AstBuilderTest, since that class is pretty large/unwieldy.

Related Issues

N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Swiddis
top and head: allow limit keywords
1c9bcb6 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis
Add docs
a42adf7 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis Swiddis added the enhancement New feature or request label Sep 8, 2025
RyanL1997
RyanL1997 approved these changes Sep 8, 2025
View reviewed changes

@RyanL1997 RyanL1997 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, in generally LGTM.

Comment thread ppl/src/test/java/org/opensearch/sql/ppl/AstPlanningTest.java Outdated
@Swiddis
Fix style + Ryan's comments
81761bc 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
ykmr1224
ykmr1224 previously approved these changes Sep 9, 2025
View reviewed changes
Comment thread docs/user/ppl/cmd/head.rst Outdated
Comment thread docs/user/ppl/cmd/top.rst Outdated
@Swiddis
Update docs
b080a61 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis
Fix version header type
98f5343 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
ykmr1224
ykmr1224 previously approved these changes Sep 26, 2025
View reviewed changes
@Swiddis
Merge branch 'main' into feature/headtop-limit
1be0c6a 
Signed-off-by: Simeon Widdis <sawiddis@gmail.com>
@Swiddis
Merge branch 'main' into feature/headtop-limit
04c9396 
Signed-off-by: Simeon Widdis <sawiddis@gmail.com>
@opensearch-trigger-bot

opensearch-trigger-bot Bot commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

This PR is stalled because it has been open for 2 weeks with no activity.

@Swiddis
Merge branch 'main' into feature/headtop-limit
de5b1a7 
Signed-off-by: Simeon Widdis <sawiddis@gmail.com>
@opensearch-trigger-bot

opensearch-trigger-bot Bot commented Nov 15, 2025

Copy link
Copy Markdown
Contributor

This PR is stalled because it has been open for 2 weeks with no activity.

@Swiddis
Merge remote-tracking branch 'upstream/main' into feature/headtop-limit
774a4d6
@Swiddis
Fmt
f1a85de 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis
Fix doctest
5518949 
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@opensearch-trigger-bot

opensearch-trigger-bot Bot commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

This PR is stalled because it has been open for 2 weeks with no activity.

@Swiddis

Swiddis commented Dec 5, 2025

Copy link
Copy Markdown
Collaborator Author

Closing since this has stalled multiple times, repeatedly gets merge conflicts, & isn't urgent -- can reopen later if it comes up again

@Swiddis Swiddis closed this Dec 5, 2025
@Swiddis Swiddis reopened this May 26, 2026
@Swiddis

Swiddis commented May 26, 2026

Copy link
Copy Markdown
Collaborator Author

it came up again.

@github-actions

github-actions Bot commented May 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 16ffe92)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Refactor test base class extraction

Relevant files:

  • ppl/src/test/java/org/opensearch/sql/ppl/AstPlanningTestBase.java
  • ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java
  • ppl/src/test/java/org/opensearch/sql/ppl/parser/AstEquivalenceTest.java
  • ppl/src/test/java/org/opensearch/sql/ppl/rewrite/SpathRewriteTest.java
Sub-PR theme: Add limit keyword support to head and top commands

Relevant files:

  • ppl/src/main/antlr/OpenSearchPPLParser.g4
  • docs/user/ppl/cmd/head.md
  • docs/user/ppl/cmd/top.md
⚡ No major issues detected

@github-actions

github-actions Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Enforce LIMIT requires EQUAL sign

The grammar allows LIMIT without EQUAL to be parsed, which could lead to ambiguous
parsing. Ensure that when LIMIT is present, EQUAL must follow to avoid parsing
conflicts with the optional integer literal.

ppl/src/main/antlr/OpenSearchPPLParser.g4 [345-347]

 headCommand
-   : HEAD ((LIMIT EQUAL)? number = integerLiteral)? (FROM from = integerLiteral)?
+   : HEAD ((LIMIT EQUAL number = integerLiteral) | number = integerLiteral)? (FROM from = integerLiteral)?
    ;
Suggestion importance[1-10]: 7

__

Why: The current grammar ((LIMIT EQUAL)? number = integerLiteral)? allows LIMIT without EQUAL, which could create ambiguity. The improved code enforces that when LIMIT is present, EQUAL must follow, making the grammar more explicit and preventing potential parsing conflicts.

Medium

@github-actions

github-actions Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 16ffe92

@penghuo penghuo merged commit e90692c into opensearch-project:main May 26, 2026
39 of 40 checks passed
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request May 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@penghuo penghuo penghuo approved these changes

@ahkcs ahkcs ahkcs approved these changes

@ykmr1224 ykmr1224 ykmr1224 left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm

@derek-ho derek-ho Awaiting requested review from derek-ho

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar vamsimanohar is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

@yuancu yuancu Awaiting requested review from yuancu yuancu is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

@songkant-aws songkant-aws Awaiting requested review from songkant-aws songkant-aws is a code owner

Assignees

No one assigned

Labels

enhancement New feature or request stalled

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Swiddis @penghuo @ykmr1224 @noCharger @ahkcs @RyanL1997